Skip to content

Specify --target-port on docs and match port on quickstart guide #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jul 10, 2025
Merged

Specify --target-port on docs and match port on quickstart guide #47

merged 3 commits into from
Jul 10, 2025

Conversation

@aponcedeleonch
Copy link
Member

@aponcedeleonch aponcedeleonch commented Jul 10, 2025

AFAIK we have 2 ports: the proxy port in which ToolHive runs and a hardcoded port that may be used by an MCP server. For the second one, the current flag should be --target-port instead of --port. Changed the only reference I could find in docs.

Also, there was a small typo in the quickstart guide in the ports. The text was talking about port 49226 but the port shown in the example was port 15266.

@aponcedeleonch
Specify --target-port on docs and match port on quickstart guide
136777c 
AFAIK we have 2 ports: the proxy port in which ToolHive runs and
a hardcoded port that may be used by an MCP server. For the second
one, the current flag should be `--target-port` instead of `--port`.
Changed the only reference I could find in docs.

Also, there was a small typo in the quickstart guide in the ports.
The text was talking about port `49226` but the port shown in the
example was port `15266`.
@aponcedeleonch aponcedeleonch requested a review from danbarr July 10, 2025 10:11
@vercel
Copy link

vercel bot commented Jul 10, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2025 0:36am

@eleftherias
Copy link
Member

eleftherias commented Jul 10, 2025

I'm not sure about this. I think the docs are correct. The --target-port is the port that the container exposes, but --port is the port that the MPC server (proxy) runs on.

Also for the 49226 and 15266, one is the container port and one is the proxy port, so I think they're meant to be different.

@aponcedeleonch
Copy link
Member Author

aponcedeleonch commented Jul 10, 2025

@eleftherias Yeah, also not sure about --target-port. All the other mentions of --port clearly refer to the proxy so left those untouched, this one seems to be the only one that could refer to the port of the container. So if it's not this one then there is no documentation on the port of the container.

Yes, you're right, I changed one port that I shouldn't, i.e. it was meant to be different. However, I do think the mention of the port in the text needed to be fixed

danbarr
danbarr requested changes Jul 10, 2025
View reviewed changes
Copy link
Collaborator

@danbarr danbarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aponcedeleonch. As you and @eleftherias were discussing, --port was indeed the intended flag in the run guide, but you're right the accompanying text wasn't as clear as it could be. Left a suggested change.

And nice catch on the port mismatch in the quickstart. I refreshed the example output recently to match a change in what the CLI shows, but missed this mismatch. Thanks!

@danbarr
Copy link
Collaborator

danbarr commented Jul 10, 2025

Fun fact, I just saw this flag is about to change on the next release: stacklok/toolhive#1022

😆

danbarr
danbarr approved these changes Jul 10, 2025
View reviewed changes
Copy link
Collaborator

@danbarr danbarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@aponcedeleonch
Copy link
Member Author

aponcedeleonch commented Jul 10, 2025

@danbarr Nice! Accepted the code suggestions. Then, should we document also the flag --target-port somewhere? I couldn't find documentation about it. Although I don't know what's the best practice, i.e. if we should document every single available flag

@aponcedeleonch aponcedeleonch merged commit cccb203 into main Jul 10, 2025
5 checks passed
@aponcedeleonch aponcedeleonch deleted the port-fixes branch July 10, 2025 12:38
@danbarr
Copy link
Collaborator

danbarr commented Jul 10, 2025

Then, should we document also the flag --target-port somewhere? I couldn't find documentation about it. Although I don't know what's the best practice, i.e. if we should document every single available flag

IMO this is more of an advanced/internal thing to need to specify the target port, for now I think we can leave this to the CLI references vs. trying to document every possible scenario. Maybe a separate "advanced configuration" guide could emerge later for less common instances like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danbarr danbarr danbarr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aponcedeleonch @eleftherias @danbarr